-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: refactoring and solves #2321 #2380
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2380 +/- ##
==========================================
- Coverage 92.36% 92.34% -0.03%
==========================================
Files 128 128
Lines 4244 4244
Branches 867 867
==========================================
- Hits 3920 3919 -1
- Misses 324 325 +1
|
saved as draft until a result merge output from #2336 in short this commit has tests that pass
when "sendBeacon" is available
when "sendBeacon" is NOT available
when "sendBeacon" is available
|
Before you take this PR out of draft status please give it a meaningful name. The PR name goes into the git log as the main message when the PR is squashed and having to refer to an external issue to learn its meaning is a frustrating workflow. |
@@ -33,6 +33,12 @@ export abstract class CollectorExporterBrowserBase< | |||
ExportItem, | |||
ServiceRequest | |||
> { | |||
private DEFAULT_HEADERS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you think it's better to keep defaults on instance instead in utils ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if i am wrong.
Who owns the config ?
- The
CollectorExporterBrowserBase
- Owning the data means working with the data and correcting the data if needed, like applying default headers or type for blob property bag.
What are the methods sendWithXhr
, sendWithBeacon
under
opentelemetry-js/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts ?
- Are roles with the responsibility to send data with a protocol.
- Protocol is the name of each function and data is the arguments that each function define.
Accept: 'application/json', | ||
'Content-Type': 'application/json', | ||
}; | ||
private DEFAULT_BLOB_TYPE = { type: 'application/json' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imho it should not be a part of CollectorExporterBrowserBase.ts as it is only used if beacon is used -> move to util
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic deciding if beacon is used , is a decision that CollectorExporterBase
already has made before sendWithBeacon util function.
{}, | ||
parseHeaders(config.headers), | ||
baggageUtils.parseKeyPairsIntoRecord( | ||
this._headers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have 2 util functions
- prepareXHRHeaders / getXHRHeaders / parseXHRHeaders etc.
- prepareBeaconHeaders ...
Currently the logic will be scattered between util and exporter, where exporter should not contain anymore logic then it is necessary and those functions will be straightforward and easy to understand. If someone want to change it you will change util class not the exporter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i think that the correct place would be to put the utility functions like the ones you mention here under
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/util.ts
What do you think ?
My motivation is because in this file there is already a helper method parseheaders
.
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/util.ts#L23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a better one, what do you think ?
rename the file util
to send
under https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
and export from it the functions sendWithXhr
, sendWithBeacon
.
Then introduce a util.ts under opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/
with the functions as helpers you mentioned above
- prepareXHRHeaders / getXHRHeaders / parseXHRHeaders etc.
- prepareBeaconHeaders ...
Then send module and util module can be imported to CollectorExporterBase
and collector exporter base can work with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obecny a more consistent solution is given by @MSNev in context of blob and sendWithBeacon here:
#2336 (comment)
So can i move the default header for xhr inside the send method of CollectorExporterBase
?
i mean this logic under util.ts
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts#L59
to send method here:
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts#L94
it should then look something like this:
if(this.xhr){
const headers = {...{"Content-Type":"application/json", "Accept": "application/json"},...this.headers}
sendWithXhr(body, this.url, headers, _onSuccess, _onError);
}
i would then close this PR and open a new one in order to be in context of xhr only , given the blob context PR will be handled here: #2336
@obecny , @MSNev, what do you think for the above refactoring proposal for xhr ?
closing this PR because of #2336 |
Which problem is this PR solving?
CollectorExporterBrowserBase
,utils
and test cases forCollectorTraceExporter
CollectorTraceExporter
Short description of the changes
CollectorExporterBrowserBase
knows about headers and logic